fix: Remove extra @objc from SessionTracker #6993
Conversation
noahsmartin
requested review from
itaybre,
philipphofmann and
philprime
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| /// Tracks sessions for release health. For more info see: | ||
| /// https://docs.sentry.io/workflow/releases/health/#session | ||
| @_spi(Private) @objc(SentrySessionTracker) public final class SessionTracker: NSObject { | ||
| final class SessionTracker { |
There was a problem hiding this comment.
Bug: Removing NSObject breaks NotificationCenter selector-based observation
The SessionTracker class no longer inherits from NSObject, but it still uses #selector with NotificationCenter.addObserver(_:selector:name:object:) for lifecycle notifications. Selector-based notification observation requires the observer to inherit from NSObject because the Objective-C runtime needs methods like methodSignatureForSelector: to dispatch the selector. Without NSObject inheritance, the notification callbacks (didBecomeActive, willResignActive, willTerminate) will fail at runtime, breaking session tracking functionality.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6993 +/- ##
=============================================
- Coverage 85.047% 85.043% -0.004%
=============================================
Files 453 453
Lines 27640 27640
Branches 12135 12132 -3
=============================================
- Hits 23507 23506 -1
- Misses 4089 4090 +1
Partials 44 44
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
philprime
left a comment
philprime
left a comment
There was a problem hiding this comment.
LGTM, but please double-check if the cursor feedback for select-based observation is valid.
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5c2d5d6 | 1225.39 ms | 1259.45 ms | 34.06 ms |
| 3b059e5 | 1201.31 ms | 1229.61 ms | 28.30 ms |
| d1c4916 | 1236.25 ms | 1266.76 ms | 30.51 ms |
| e70670c | 1223.47 ms | 1238.67 ms | 15.20 ms |
| 99ec589 | 1209.73 ms | 1231.98 ms | 22.24 ms |
| 458a7d6 | 1212.67 ms | 1244.90 ms | 32.23 ms |
| 1a34ddc | 1218.94 ms | 1251.86 ms | 32.92 ms |
| 605fa27 | 1226.31 ms | 1251.35 ms | 25.05 ms |
| 1ed7bf6 | 1215.78 ms | 1255.78 ms | 40.00 ms |
| f8687d1 | 1227.54 ms | 1258.44 ms | 30.90 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5c2d5d6 | 23.75 KiB | 969.28 KiB | 945.54 KiB |
| 3b059e5 | 23.75 KiB | 980.73 KiB | 956.99 KiB |
| d1c4916 | 23.75 KiB | 981.15 KiB | 957.40 KiB |
| e70670c | 23.75 KiB | 975.19 KiB | 951.45 KiB |
| 99ec589 | 23.75 KiB | 983.31 KiB | 959.57 KiB |
| 458a7d6 | 23.75 KiB | 1.01 MiB | 1016.13 KiB |
| 1a34ddc | 23.75 KiB | 919.88 KiB | 896.13 KiB |
| 605fa27 | 23.75 KiB | 908.03 KiB | 884.28 KiB |
| 1ed7bf6 | 24.14 KiB | 1.01 MiB | 1012.91 KiB |
| f8687d1 | 23.75 KiB | 988.01 KiB | 964.27 KiB |
noahsmartin
force-pushed
the
removeSomeObjC
branch
from
3e1852a to
27b6013
Compare
noahsmartin
force-pushed
the
removeSomeObjC
branch
from
27b6013 to
9a0b87e
Compare
| @objcMembers public class TestNSNotificationCenterWrapper: NSObject { | ||
| private enum Observer { | ||
| case observerWithObject(WeakReference<NSObject>, Selector, NSNotification.Name?, Any?) | ||
| case observerWithObject(WeakReference<AnyObject>, Selector, NSNotification.Name?, Any?) |
There was a problem hiding this comment.
Bug: Observer removal fails for non-NSObject observers in test utility
The Observer.observerWithObject case was changed to use WeakReference<AnyObject> to support non-NSObject observers, but the removeObserver method's comparison logic at line 85 still uses observer as? NSObject. For non-NSObject observers, this cast returns nil, causing the identity comparison weakObserver.value === observer as? NSObject to fail, so observers won't be properly removed. The comparison should use observer as AnyObject instead.
Follow up from #6985
#skip-changelog
Closes #6994